core: make target manager accessible on driver #14122

brendankenny · 2022-06-13T20:15:08Z

Should help significantly with #14078. Smoke tests should all pass, but waiting on feedback before touching the unit test mocks :)

This PR elevates TargetManager to a top-level property on Driver. The intention is that TargetManager

will be the sole place tracking and attaching to targets/sessions (including wrapping CDPSessions in FR sessions), which should make the execution flow easier to understand and make future changes simpler.
will live for as long as Driver does, which includes across passes. It's basically a live view of all the sessions Lighthouse knows about
the place to subscribe to all protocol events (with the flattened sessionId included)

The legacy driver basically does these things already, so it was easy to expose a shim on it and keep NetworkMonitor (and its users) on a unified implementation. It's not pretty architecturally, but it is simple (yay) and should only be around until LH 11.

Since TargetManager tracks all sessions and their targetInfo, in the future it could be used for things like, "give me all the iframe sessions" so you can send commands or evaluate() in them easily, but nothing needs that right now and it's harder to include legacy support, so we can wait.

re: @paulirish's #14120 (comment), AFAICT all sessions now have IDs, presumably because pptr is keeping the sessionId-less session to itself. I'd like to verify that and move us to a place where sessionId is required in the all-protocol-events type:

lighthouse/types/protocol.d.ts

Lines 68 to 69 in 1d6ee0e

    
           // If sessionId is not set, it means the event was from the root target. 
        
           sessionId?: string;

so code will hopefully be forced to contend with it. Meanwhile in this PR, session drops the sessionId so it's like CDPSession. If you're subscribing to a single session's events, there's no need for a session ID. We can follow up and make the session event handling simpler, but I didn't want the changes in this PR interleaved with that change.

brendankenny · 2022-06-13T20:18:13Z

lighthouse-core/fraggle-rock/gather/driver.js

+    const cdpSession = await this._page.target().createCDPSession();
+    this._targetManager = new TargetManager(cdpSession);
+    await this._targetManager.enable();
+    this.defaultSession = this._targetManager.rootSession();


Because TargetManager is in charge of creating FR ProtocolSessions from CDPSessions, and we want to eventually make those sessions available for gatherers to access directly, Driver now gets its driver.defaultSession from targetManager instead of making it itself. This is a little weird, but otherwise we have two different session objects pointing to the same thing. This seemed conceptually simpler (driver setup is a little more involved but it means the set of all active sessions includes defaultSession) but open to ideas.

Big fan of avoiding the double session object! (I was seeing that at the CDP traffic level. so weird.)

brendankenny · 2022-06-13T20:19:37Z

types/protocol.d.ts

+   *   'Domain.method2Name': {method: 'Domain.method2Name', params: EventPayload2},
+   * }
+   */
+  type RawEventMessageRecord = {


this is an unchanged move from below, but it was previously scoped to the file and we need it exported now

brendankenny · 2022-06-13T20:20:57Z

lighthouse-core/gather/gatherers/devtools-log.js

-    this._networkMonitor.on('protocolmessage', this._onProtocolMessage);
-    this._networkMonitor.enable();
+    driver.targetManager.on('protocolevent', this._onProtocolMessage);
+    await driver.defaultSession.sendCommand('Page.enable');


we have so many Page.enables in LH that this has to be unnecessary, but this matches the behavior of the this._networkMonitor.enable() call

connorjclark

(first pass)

lighthouse-core/gather/driver/target-manager.js

lighthouse-core/gather/driver.js

lighthouse-core/gather/driver/target-manager.js

paulirish

lots of great improvements here. thanks for taking this on.

paulirish · 2022-06-13T20:46:01Z

lighthouse-core/fraggle-rock/gather/driver.js

+    const cdpSession = await this._page.target().createCDPSession();
+    this._targetManager = new TargetManager(cdpSession);
+    await this._targetManager.enable();
+    this.defaultSession = this._targetManager.rootSession();


Big fan of avoiding the double session object! (I was seeing that at the CDP traffic level. so weird.)

paulirish · 2022-06-13T20:47:06Z

lighthouse-core/fraggle-rock/gather/driver.js

+    const cdpSession = await this._page.target().createCDPSession();
+    this._targetManager = new TargetManager(cdpSession);
+    await this._targetManager.enable();
+    this.defaultSession = this._targetManager.rootSession();


i'd love to s/defaultSession/rootSession/g but fine with it happening in a followup.

paulirish · 2022-06-13T20:56:51Z

lighthouse-core/gather/driver/target-manager.js


-      await session.sendCommand('Target.setAutoAttach', {
+      // We want to receive information about network requests from iframes, so enable the Network domain.
+      await newSession.sendCommand('Network.enable');


do you think we should also tweak this to do a promise.all style? 4bc0b05
(im still surprised this is fine without it)
also.. i wonder if the finally case hits because of a protocol_timeout on one of these?

i know you saw some patterns in pptr/playwright which felt more confident around this stuff. dunno if that's relevant.

adamraine · 2022-06-15T21:54:09Z

types/gatherer.d.ts

+    targetManager: {
+      rootSession(): FRProtocolSession;
+      on(event: 'protocolevent', callback: (payload: Protocol.RawEventMessage) => void): void
+      off(event: 'protocolevent', callback: (payload: Protocol.RawEventMessage) => void): void
+    };


Could give this its own interface. Then the target manager and legacy shim will implement that interface.

Might help with some of the event type TS stuff you did in target-manager.js

I'm hoping that it'll stay nice and simple until LH 11, and then needing an interface won't even be a question with legacy gone, it'll just be itself.

adamraine · 2022-06-15T22:02:32Z

lighthouse-core/gather/gatherers/devtools-log.js

 const FRGatherer = require('../../fraggle-rock/gather/base-gatherer.js');

+/** @typedef {import('../driver/target-manager.js')} TargetManager */


Where is this used?

Where is this used?

it's not, deleted :)

brendankenny · 2022-06-21T17:54:37Z

Addressed feedback and unit tests updated. Test updates are in separate commits to make it slightly easier to review.

Let's add a mock simplification pass to our project TODOs, that was way harder (and more than 2x the lines changed) than refactoring the actual code :)

brendankenny · 2022-06-21T17:58:45Z

lighthouse-core/test/fraggle-rock/gather/session-test.js

@@ -31,58 +31,6 @@ describe('ProtocolSession', () => {
    session = new ProtocolSession(puppeteerSession);
  });

-  describe('ProtocolSession', () => {


tests in here moved to target-manager-test.js

connorjclark · 2022-06-22T00:42:59Z

Should help significantly with #14078. Smoke tests should all pass, but waiting on feedback before touching the unit test mocks :)

This seems to close out the linked issue (I'm inferring from this quoted text that there is more to do, but can't tell what).

brendankenny · 2022-06-22T16:20:30Z

Should help significantly with #14078. Smoke tests should all pass, but waiting on feedback before touching the unit test mocks :)

This seems to close out the linked issue (I'm inferring from this quoted text that there is more to do, but can't tell what).

I think mostly, but I believe @paulirish has a few more things adjacent to this, like settling on some naming conventions across files, using regular event emitters where possible, etc. We could discuss in #14078 and/or next sync.

brendankenny requested a review from a team as a code owner June 13, 2022 20:15

brendankenny requested review from connorjclark and removed request for a team June 13, 2022 20:15

brendankenny commented Jun 13, 2022

View reviewed changes

devtools-bot assigned connorjclark Jun 13, 2022

devtools-bot added the waiting4reviewer label Jun 13, 2022

brendankenny commented Jun 13, 2022

View reviewed changes

connorjclark reviewed Jun 13, 2022

View reviewed changes

paulirish reviewed Jun 13, 2022

View reviewed changes

connorjclark changed the title ~~core: make targetManager accessible on driver~~ core: make target manager accessible on driver Jun 13, 2022

connorjclark added waiting4committer and removed waiting4reviewer labels Jun 14, 2022

adamraine reviewed Jun 15, 2022

View reviewed changes

brendankenny added 7 commits June 21, 2022 12:50

core: make targetManager accessible on driver

06a655c

feedback

ccc846f

target-manager-test

b3a55b2

move protocolevent tests from session to target-manager

f78707e

network-monitor-test

a3bc0c7

driver-test

f4d4c8c

full-page-screenshot-test

39b9d64

brendankenny force-pushed the tm-on-driver branch from b24fc22 to 39b9d64 Compare June 21, 2022 17:51

vercel bot deployed to Preview June 21, 2022 17:52 View deployment

brendankenny added waiting4reviewer and removed waiting4committer labels Jun 21, 2022

brendankenny commented Jun 21, 2022

View reviewed changes

connorjclark approved these changes Jun 22, 2022

View reviewed changes

brendankenny merged commit a5434c8 into master Jun 22, 2022

brendankenny deleted the tm-on-driver branch June 22, 2022 16:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: make target manager accessible on driver #14122

core: make target manager accessible on driver #14122

brendankenny commented Jun 13, 2022

brendankenny Jun 13, 2022 •

edited

Loading

paulirish Jun 13, 2022

brendankenny Jun 13, 2022

brendankenny Jun 13, 2022

connorjclark left a comment

paulirish left a comment

paulirish Jun 13, 2022

paulirish Jun 13, 2022

paulirish Jun 13, 2022

adamraine Jun 15, 2022

brendankenny Jun 21, 2022

adamraine Jun 15, 2022

brendankenny Jun 21, 2022

brendankenny commented Jun 21, 2022 •

edited

Loading

brendankenny Jun 21, 2022 •

edited

Loading

connorjclark commented Jun 22, 2022 •

edited

Loading

brendankenny commented Jun 22, 2022

	// If sessionId is not set, it means the event was from the root target.
	sessionId?: string;

		const FRGatherer = require('../../fraggle-rock/gather/base-gatherer.js');

		/** @typedef {import('../driver/target-manager.js')} TargetManager */

core: make target manager accessible on driver #14122

core: make target manager accessible on driver #14122

Conversation

brendankenny commented Jun 13, 2022

brendankenny Jun 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark left a comment

Choose a reason for hiding this comment

paulirish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Jun 21, 2022 • edited Loading

brendankenny Jun 21, 2022 • edited Loading

Choose a reason for hiding this comment

connorjclark commented Jun 22, 2022 • edited Loading

brendankenny commented Jun 22, 2022

brendankenny Jun 13, 2022 •

edited

Loading

brendankenny commented Jun 21, 2022 •

edited

Loading

brendankenny Jun 21, 2022 •

edited

Loading

connorjclark commented Jun 22, 2022 •

edited

Loading